[PM-17168] Sync organization user revoked/restored status immediately via push notification #13101

r-tome · 2025-01-28T12:43:34Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-17168

📔 Objective

Revoking/restoring user is not synced immediately via push notification

This PR updates the NotificationsService to handle messages of type NotificationType.SyncOrgKeys by refreshing the identity token.

I also noticed that the SignalR connection stops when receiving a NotificationType.SyncOrgKeys message but does not restart automatically. To fix this, I updated the reconnect method to re-setup the connection.

Server PR.

📸 Screenshots

Revoking and restoring a user a couple of times:

Screen.Recording.2025-01-28.at.12.41.05.mov

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

…wait for reconnect

… in a separate method

…tionsService

…diately-via-push-notification

…OrgKeysOnRevokeRestore

github-actions · 2025-01-28T12:51:45Z

Checkmarx One – Scan Summary & Details – 24a6f130-1359-40da-b0bc-f708778a8eb5

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	Client_Privacy_Violation	/apps/desktop/src/app/tools/password-generator-history.component.html: 15	details Method password_generator_history_component at line 15 of /apps/desktop/src/app/tools/password-generator-history.component.html sends user informat... Attack Vector

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	Angular_Usage_of_Unsafe_DOM_Sanitizer	/libs/components/src/avatar/avatar.component.ts: 87

codecov · 2025-01-28T13:03:21Z

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 35.04%. Comparing base (72434bf) to head (57d9548).
Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
...ications/internal/default-notifications.service.ts	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13101      +/-   ##
==========================================
- Coverage   35.41%   35.04%   -0.38%     
==========================================
  Files        3007     3034      +27     
  Lines       90716    92642    +1926     
  Branches    16858    16859       +1     
==========================================
+ Hits        32129    32463     +334     
- Misses      56123    57719    +1596     
+ Partials     2464     2460       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

libs/common/src/services/notifications.service.ts

r-tome · 2025-01-30T11:22:41Z

I'm closing this because only the server change is required.

…diately-via-push-notification

… enabled

r-tome · 2025-02-03T14:55:39Z

Reopening this PR — @eliykat explained that we need to refresh the identity token to update the claims.

libs/common/src/platform/notifications/internal/default-notifications.service.ts

…diately-via-push-notification

libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts

justindbaur

Thanks!

justindbaur

Retracting approval for a moment while I think about this behavior on top of how SyncService.fullSync works.

justindbaur · 2025-02-05T17:38:46Z

libs/common/src/platform/notifications/internal/default-notifications.service.ts

+        if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) {
+          // Refresh the identity token to ensure organization roles in JWT claims are up-to-date
+          await this.apiService.refreshIdentityToken();
+        }


I'm actually going to keep the rejection only because I think your server change should be enough for now. SyncService.fullSync(true) will automatically call ApiService.refreshIdentityToken(). We are thinking about decoupling that behavior and the change here you have would be right but we are not going to be merging that right now. So please check my assumption here but you should get the desired outcome with only the server PR. I've linked the tickets and mentioned the PR on our decoupling work so that if we go ahead with that we will add the ApiService.refreshIdentityToken call for you.

I didn't know fullSync already did this for us, so fwiw I'm OK with that suggestion. Thanks @justindbaur

So please check my assumption here but you should get the desired outcome with only the server PR

Yes that is true, the server PR changes alone are enough. Thanks for looking into this, @justindbaur!

I'll close this PR.

r-tome · 2025-02-06T10:14:24Z

Closing the PR, see the reason here.

r-tome added 9 commits January 27, 2025 15:42

Refresh identity token on SyncOrgKeys notification message

4ddfc1e

Fix floating promise in signalrConnection onclose handler by adding a…

1398c49

…wait for reconnect

Refactor NotificationsService to encapsulate SignalR connection setup…

cfe0ea4

… in a separate method

Set up SignalR connection on reconnect

8acbc16

Add PushSyncOrgKeysOnRevokeRestore feature flag

fffbb5b

Add feature flag check for PushSyncOrgKeysOnRevokeRestore in Notifica…

f4e4625

…tionsService

Merge branch 'main' into ac/pm-17168/revoking-user-is-not-synced-imme…

4a4e35c

…diately-via-push-notification

Add ConfigService for NotificationsService in jslib-services.module.ts

934791c

Update NotificationsService to await feature flag checks for PushSync…

839eacc

…OrgKeysOnRevokeRestore

r-tome changed the title ~~[PM17168] Sync organization user revoked/restored status immediately via push notification~~ [PM-17168] Sync organization user revoked/restored status immediately via push notification Jan 28, 2025

r-tome mentioned this pull request Jan 28, 2025

[PM-17168] Sync organization user revoked/restored status immediately via push notification bitwarden/server#5330

Merged

r-tome requested review from a team and BTreston January 28, 2025 12:51

Add ConfigService to MainBackground

ff7eef9

r-tome marked this pull request as ready for review January 28, 2025 14:28

eliykat reviewed Jan 30, 2025

View reviewed changes

libs/common/src/services/notifications.service.ts Outdated Show resolved Hide resolved

r-tome closed this Jan 30, 2025

r-tome added 2 commits February 3, 2025 11:45

Merge branch 'main' into ac/pm-17168/revoking-user-is-not-synced-imme…

eb30531

…diately-via-push-notification

Refresh identity token on SyncOrgKeys notification if feature flag is…

9f6ecfb

… enabled

r-tome reopened this Feb 3, 2025

r-tome requested a review from a team as a code owner February 3, 2025 14:49

r-tome requested a review from tangowithfoxtrot February 3, 2025 14:49

BTreston previously approved these changes Feb 3, 2025

View reviewed changes

eliykat previously approved these changes Feb 3, 2025

View reviewed changes

eliykat reviewed Feb 3, 2025

View reviewed changes

libs/common/src/platform/notifications/internal/default-notifications.service.ts Show resolved Hide resolved

Add comment to clarify why the identity token needs to be refreshed

1764a8b

r-tome dismissed stale reviews from eliykat and BTreston via 1764a8b February 4, 2025 09:33

r-tome requested a review from BTreston February 4, 2025 09:33

Merge branch 'main' into ac/pm-17168/revoking-user-is-not-synced-imme…

3b144ba

…diately-via-push-notification

eliykat previously approved these changes Feb 4, 2025

View reviewed changes

justindbaur requested changes Feb 5, 2025

View reviewed changes

libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts Show resolved Hide resolved

Add mocks for ApiService and ConfigService in NotificationsService tests

57d9548

r-tome dismissed eliykat’s stale review via 57d9548 February 5, 2025 16:26

r-tome requested a review from justindbaur February 5, 2025 16:26

justindbaur approved these changes Feb 5, 2025

View reviewed changes

justindbaur requested changes Feb 5, 2025

View reviewed changes

r-tome closed this Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-17168] Sync organization user revoked/restored status immediately via push notification #13101

[PM-17168] Sync organization user revoked/restored status immediately via push notification #13101

r-tome commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

r-tome commented Jan 30, 2025

r-tome commented Feb 3, 2025

justindbaur left a comment

justindbaur left a comment

justindbaur Feb 5, 2025

eliykat Feb 6, 2025

r-tome Feb 6, 2025

r-tome commented Feb 6, 2025

[PM-17168] Sync organization user revoked/restored status immediately via push notification #13101

[PM-17168] Sync organization user revoked/restored status immediately via push notification #13101

Conversation

r-tome commented Jan 28, 2025 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Jan 28, 2025 • edited Loading

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

r-tome commented Jan 30, 2025

r-tome commented Feb 3, 2025

justindbaur left a comment

Choose a reason for hiding this comment

justindbaur left a comment

Choose a reason for hiding this comment

justindbaur Feb 5, 2025

Choose a reason for hiding this comment

eliykat Feb 6, 2025

Choose a reason for hiding this comment

r-tome Feb 6, 2025

Choose a reason for hiding this comment

r-tome commented Feb 6, 2025

r-tome commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading